Codecov Report

❌ Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.40%. Comparing base (8ca225b) to head (25b233a).

@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
- Coverage   86.74%   86.40%   -0.35%     
==========================================
  Files          12       12              
  Lines        2369     2383      +14     
==========================================
+ Hits         2055     2059       +4     
- Misses        238      247       +9     
- Partials       76       77       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review for PR #577

Summary

This PR addresses issue #430 by preventing the control connection from being closed due to idle timeout when there's an active data transfer in progress. The solution is straightforward and correctly identifies active transfers before deciding whether to close the connection.

✅ Strengths

• Correct fix: The solution properly addresses the core issue where large/slow transfers could be interrupted by idle timeout
• Thread-safe: Uses proper mutex locking when checking isTransferOpen state
• Minimal change: Focused fix that doesn't introduce unnecessary complexity
• Follows existing patterns: Uses the same deadline extension pattern already present in the codebase

🔍 Issues and Recommendations

1. Potential Infinite Loop Risk 🚨

The current implementation extends the deadline by IdleTimeout seconds each time. If a transfer stalls (e.g., network issues, client hanging), this could keep the connection alive indefinitely.

Recommendation: Consider adding a maximum transfer timeout or counter to prevent infinite extensions:

// Add a field to track consecutive extensions
if hasActiveTransfer {
    if c.transferTimeoutExtensions >= maxTransferExtensions {
        // Force close after too many extensions
        c.logger.Warn("Transfer timeout exceeded maximum extensions")
        // Continue with normal timeout handling
    } else {
        c.transferTimeoutExtensions++
        // Extend deadline...
    }
}

2. Missing Test Coverage ⚠️

There are no tests for this new behavior. The existing TestIdleTimeout doesn't cover the scenario where a transfer is active.

Recommendation: Add a test case that:

• Initiates a slow transfer (using throttled connection or large file)
• Verifies the control connection remains open beyond IdleTimeout
• Confirms connection closes after transfer completes

3. Deadline Calculation Complexity 🔧

The deadline calculation is unnecessarily complex:

time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout))

Simplification:

time.Duration(c.server.settings.IdleTimeout) * time.Second

4. Reset Transfer Extension Counter 📝

When a transfer completes successfully, any transfer extension counter should be reset for the next transfer (in the closeTransfer method at client_handler.go:339).

🔒 Security Considerations

• The fix doesn't introduce new attack vectors
• However, consider adding rate limiting or maximum transfer duration to prevent resource exhaustion attacks where clients intentionally keep slow transfers running

📊 Performance Impact

• Minimal overhead: One mutex lock/unlock per timeout event during active transfers
• No impact on normal operations without active transfers

📚 Documentation

Consider updating documentation to clarify:

• How IdleTimeout behaves during active transfers
• Any maximum transfer duration limits (if implemented)

✅ Overall Assessment

The PR correctly fixes the reported issue with a clean, focused change. However, it needs:

1. Test coverage for the new behavior
2. Protection against infinite deadline extensions
3. Minor code simplification for readability

Once these items are addressed, this will be a solid fix for the idle timeout issue during active transfers.

Code Review for PR #577: Fix IdleTimeout during active transfers

Thank you for this fix! I've reviewed the changes and have the following feedback:

✅ Strengths

• Correctly addresses the issue: The fix properly checks for active data transfers before closing the control connection on idle timeout
• Thread-safe implementation: Proper use of mutex lock/unlock to access isTransferOpen
• Maintains existing behavior: When no transfer is active, the original timeout handling is preserved
• Good defensive coding: Includes error handling for SetDeadline failures

🔍 Areas for Improvement

1. Deadline Extension Logic

The current implementation has a potential issue with the deadline extension calculation:

extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))

Issues:

• The conversion is unnecessarily complex. Since IdleTimeout is already in seconds, you can simplify this to:

  extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)

• More importantly, this creates a recursive timeout loop. If the transfer takes longer than another idle timeout period, the same timeout will trigger again, extending indefinitely. Consider using a longer extension period or a maximum number of extensions.

2. Missing Test Coverage

The PR doesn't include tests for this new behavior. Consider adding a test case that:

• Sets up a server with a short idle timeout
• Initiates a large file transfer that takes longer than the idle timeout
• Verifies the connection stays open during the transfer
• Confirms the connection closes after the transfer completes and idle timeout expires

Example test structure:

func TestIdleTimeoutWithActiveTransfer(t *testing.T) {
    // Create server with 2-second idle timeout
    // Start a file transfer
    // Sleep for 3 seconds (> idle timeout)
    // Verify connection is still alive
    // Complete transfer
    // Wait for idle timeout
    // Verify connection closes
}

3. Potential Race Condition

While the mutex protects isTransferOpen, there's still a window between checking the flag and extending the deadline where the transfer could complete. This is likely acceptable given the use case, but worth noting.

4. Consider Alternative Approaches

Instead of repeatedly extending on timeout, consider:

• Disabling idle timeout entirely while a transfer is active (set to 0 or very large value when transfer starts, restore when it ends)
• Using a separate goroutine to monitor transfer status and manage deadlines
• Implementing a transfer-aware timeout that resets on data activity rather than control channel activity

📝 Suggested Implementation

Here's a more robust approach:

if hasActiveTransfer {
    // Extend by a reasonable period (e.g., 5 minutes or 10x idle timeout)
    extensionPeriod := time.Duration(c.server.settings.IdleTimeout) * time.Second * 10
    if extensionPeriod < 5*time.Minute {
        extensionPeriod = 5 * time.Minute
    }
    
    if errSet := c.conn.SetDeadline(time.Now().Add(extensionPeriod)); errSet != nil {
        c.logger.Error("Could not extend deadline during transfer", "err", errSet)
        // Consider whether to continue or close the connection here
    }
    
    if c.debug {
        c.logger.Debug("Extended deadline due to active transfer", 
            "extension", extensionPeriod)
    }
    return
}

🚀 Overall Assessment

This is a good fix for an important issue. With the suggested improvements to the deadline calculation and added test coverage, this will be a solid addition to the codebase. The fix aligns well with the library's architecture and maintains backward compatibility.

Code Review for PR #577

I've reviewed the changes to fix the IdleTimeout issue during active data transfers. Here's my feedback:

✅ Positive Aspects

1. Correct Problem Identification: The fix correctly addresses issue IdleTimeout doesn't factor in active data connection (large or slow transfers) #430 by checking for active transfers before closing the control connection on idle timeout.

2. Proper Mutex Usage: The code correctly uses transferMu.Lock() and Unlock() to safely read isTransferOpen, following the existing pattern in the codebase (client_handler.go:99).

3. Appropriate Logging: Debug logging is added to track when deadlines are extended during active transfers.

⚠️ Issues & Concerns

1. Critical Bug: Inconsistent Transfer State Management (client_handler.go:339, 676)

The isTransferOpen flag is not protected by transferMu when it's set to true (line 676) or false (line 339) in other parts of the code. This creates a race condition:

• Line 676: c.isTransferOpen = true (no lock held)
• Line 339: c.isTransferOpen = false (no lock held)
• PR adds: Lines 479-481 check isTransferOpen (lock held)

Recommendation: All reads AND writes to isTransferOpen must be protected by transferMu to prevent data races. The existing code at lines 339 and 676 should also acquire the lock.

2. Incorrect Time Calculation (Line 486)

extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))

This is unnecessarily complex and error-prone. IdleTimeout is already in seconds, so you're doing: time.Second.Nanoseconds() * seconds = nanoseconds, then converting back to time.Duration.

Recommendation: Simplify to:

extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)

3. Missing Test Coverage

There's an existing TestIdleTimeout (handle_misc_test.go:44) but no test that validates:

• Idle timeout doesn't close connection during active transfer
• Deadline is properly extended during transfers
• Normal idle timeout still works when no transfer is active

Recommendation: Add a test case like:

func TestIdleTimeoutDuringTransfer(t *testing.T) {
    // Setup server with short idle timeout
    // Start a slow file transfer
    // Verify control connection stays open past idle timeout
    // Verify deadline is extended (check logs or add metrics)
}

4. Duplicate Deadline Extension Logic (Lines 480, 486)

The code extends the deadline twice in the same error path:

• Line 480: c.conn.SetDeadline(time.Now().Add(time.Minute)) (existing code)
• Line 486: Sets deadline to IdleTimeout (new code, only when transfer active)

Recommendation: The logic flow seems confusing. Consider restructuring to:

if errNetError.Timeout() {
    c.transferMu.Lock()
    hasActiveTransfer := c.isTransferOpen
    c.transferMu.Unlock()

    if hasActiveTransfer {
        // Extend deadline for active transfer
        extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)
        if errSet := c.conn.SetDeadline(extendedDeadline); errSet != nil {
            c.logger.Error("Could not extend deadline during active transfer", "err", errSet)
        }
        if c.debug {
            c.logger.Debug("Idle timeout during active transfer, extending deadline")
        }
        return
    }

    // No active transfer, proceed with timeout handling
    if errSet := c.conn.SetDeadline(time.Now().Add(time.Minute)); errSet != nil {
        c.logger.Error("Could not set read deadline", "err", errSet)
    }
    // ... rest of timeout handling
}

🔒 Security Considerations

Potential DoS Vector: An attacker could keep a connection open indefinitely by continuously initiating transfers. The deadline is extended by IdleTimeout each time, so with a 60-second timeout and repeated transfers, a connection could stay open forever.

Recommendation: Consider adding:

• Maximum total connection duration (separate from idle timeout)
• Transfer timeout (maximum time for a single transfer)
• Limit on consecutive deadline extensions

📊 Performance Considerations

The mutex lock/unlock is lightweight and happens only during timeout events (rare). Performance impact is negligible.

📋 Summary

Must Fix Before Merge:

1. Fix race condition by protecting ALL isTransferOpen access with transferMu
2. Simplify time calculation

Should Fix:

1. Add test coverage for this scenario
2. Clarify deadline extension logic
3. Consider DoS protection

Code Quality: 6/10 (correct concept, implementation needs refinement)

🤖 Generated with Claude Code